Implement lazily created sub commands. #1276
Conversation
|
Interesting, thanks. I have quite a few details I want to work through, but think I get the idea.
In case I am missing something, I think lazy-loading and promise support are two different things? (i.e. might be desirable but not required?) |
|
It’s required for my use case now. |
|
@shadowspawn let me know if this feature is desired. See the implementation: It currently uses @npmcli/arborist (npm 7 beta) for testing purpose. |
|
Thanks for the example. |
Yes and no. I am interested in exploring the details further, but won't be accepting the PR in its current form if that is what you are asking. Due to this PR I have been doing some research and thinking on lazy loading, and promisifying, and how best to add new functionality to command creation. I will post more. For my own reference on the command creation, took me a while to find a previous PR where a similar addition was being made to the command "options" like |
|
Probably its possible to implement this feature with multiple independent commands, like you recommended at #886, but it will use allowUnknowOptions and I always like to try the upstream approach first. At https://github.com/mshima/commander.js/tree/multiple I have a implementation (poor) that allows to run multiple commands using |
|
The end result is:
|
|
Your example is a bit different than I expected. It isn't just lazy-loading, but is loading dynamic commands based on the arguments to the declared command (i.e. declared And you mentioned running multiple commands too in the following comments. These are just words so might not be clear, but are you interested in "dynamic" loading of commands, or "lazy" loading of commands? (I think your proposed implementation supports both, but some of the changes I was going to explore do not!) |
|
IMO the created command isn't dynamic, once it's created the definitions are all there. |
| if (actionPromise instanceof Promise) { | ||
| return actionPromise; | ||
| } | ||
| return Promise.resolve(actionPromise); |
There was a problem hiding this comment.
It would probably be more consistent with JavaScript conventions to check for "thenable" than for a Promise as such, but in fact can just drop the test entirely as Promise.resolve handles Promises too.
| * @api public | ||
| */ | ||
|
|
||
| parseAsync(argv, parseOptions) { | ||
| this.parse(argv, parseOptions); | ||
| return Promise.all(this._actionResults).then(() => this); | ||
| return this._parseProgram(argv, parseOptions).then(() => { |
There was a problem hiding this comment.
Instead of scattering Promise.resolve through the code in places that do not use Promises, could call Promise.resolve here instead to minimise the code churn.
|
It is encouraging that lazy loading and dynamic loading using Promises did not require too much change. However, this PR is doing too many things that each need to be tackled more carefully for easy adoption and the longterm maintenance of the program. The recursive A possible insight for future evolution of Commander API is that a key difference between an ordinary command and one implemented using a stand-alone executable is that the latter is essentially lazy loaded. The executable command name and parameters and description are all available so that the (parent) help can be generated without loading the command itself. This is also relevant for dynamic and lazy loading. I wonder whether the dynamic loading could be implemented with current code with an async action handler. Thank you for your contributions. |
Pull Request
Problem
Some programs implements plugin like sub commands.
The plugin options isn't known at the time the root commander is created.
Solution
ChangeLog